Use Karma/BrowserStack to run <model-viewer> tests #1075

cdata · 2020-03-05T17:25:43Z

This change grew substantially out of scope, but one thing led to another and now our test toolchain and CI pipeline is totally different ✨

This change updates our pipeline to run in Github Actions instead of Travis. This is good because it brings us closer to Github's well-tread cowpaths, and moves us away from Travis which seems to be more-or-less frozen in time feature/support-wise (probably because of their recent sale).

Secondly, this change moves us from SauceLabs to BrowserStack for our x-browser testing needs. This has generally yielded an increase in overall performance for our x-browser tests, as well as improved reliability for iOS Safari tests and unlocks the potential to re-introduce our Android Chrome tests in the future. We might even get crazy and add tests for Samsung Internet someday. Future work on this tracked in #691 .

Thirdly, this change moves our test toolchain from WCT (which is deprecated) to Karma (which has an uncertain future, but is way better than WCT and has active support for now). Thankfully, only minimal changes were required to our test suites to support this shift. Most of the time spent on this change went to ensuring that the combination of Karma, BrowserStack and Github Actions all worked in harmony and as intended.

And the coup de grâce: npm run dev does something useful for the @google/model-viewer package now (more or less what it does for @google/3dom), so running all the build tools simultaneously in a single terminal just got easier.

A final note on Travis: this PR continues to run Travis builds even though the Travis config has been removed. Travis has to be manually turned off for our repository before those status lines will go away. I will do that after this PR is merged.

An example 3P PR has been set up here to demonstrate how the build changes: #1082

elalish

Awesome! I remember one of the first things you told me when I joined this project was that we should purge Sauce Labs with fire. That day has finally arrived.

elalish · 2020-03-10T21:02:08Z

.github/workflows/unit-tests-legacy.yml

+        BROWSER_STACK_USERNAME: ${{ secrets.BROWSER_STACK_USERNAME }}
+        BROWSER_STACK_ACCESS_KEY: ${{ secrets.BROWSER_STACK_ACCESS_KEY }}
+      run: |
+        # Don't run Sauce from a forked repo, since the code may not be trusted


If this is still true, we should probably at least update that it's not Sauce anymore.

elalish · 2020-03-10T21:02:45Z

.github/workflows/unit-tests.yml

-        SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }}
-        SAUCE_ACCESS_KEY: ${{ secrets.SAUCE_ACCESS_KEY }}
+        BROWSER_STACK_USERNAME: ${{ secrets.BROWSER_STACK_USERNAME }}
+        BROWSER_STACK_ACCESS_KEY: ${{ secrets.BROWSER_STACK_ACCESS_KEY }}
      run: |
        # Don't run Sauce from a forked repo, since the code may not be trusted


Same as above; maybe just do a search for 'Sauce'.

elalish · 2020-03-10T21:03:14Z

.travis.yml

@@ -26,7 +26,7 @@ install:
 - npm run bootstrap
 - npm run build
 script:
- npm run test:ci
+- npm run wct-local-chrome


WCT is gone now, right?

elalish · 2020-03-10T21:04:31Z

packages/3dom/karma.conf.js

+    // Something in Karma deps actually asserts the sub-keys of Object.assign,
+    // so make sure to copy those over too:
+    assign.call(Object, newAssign, assign);
+    Object.assign = newAssign;


I love JavaScript! 😈

elalish · 2020-03-10T21:09:52Z

packages/model-viewer/karma.conf.js

+        // domain so we go there directly instead:
+        url: 'http://bs-local.com:9876'
+      },
+      'iOS Safari (iOS 12)': {


Should we add an android device someday? Not saying today is that day...

Yes, let's make it another day though. Tracked in #691

elalish · 2020-03-10T21:11:40Z

packages/model-viewer/package.json

    "focus-visible": "^5.0.1",
    "fullscreen-polyfill": "^1.0.2",
+    "http-server": "^0.12.1",


Better than local-web-server?

Yes, and consistent with the other packages

elalish · 2020-03-10T21:12:32Z

packages/model-viewer/package.json

    "intersection-observer": "^0.5.1",
    "lit-html": "^1.1.2",
-    "local-web-server": "^2.6.0",
-    "make-dir": "^1.3.0",
    "mocha": "^5.2.0",


Does karma-mocha not include mocha?

It does not, it manages the integration between the two.

elalish · 2020-03-10T21:13:56Z

packages/model-viewer/scripts/ci-run-tests.sh


-  if  [[ "${TRAVIS_PULL_REQUEST}" = "false" && "${TRAVIS_BRANCH}" != "master" ]]; then


Does this mean third parties start browserStack tests, or did this logic end up elsewhere?

This got moved elsewhere

elalish · 2020-03-10T21:15:34Z

packages/model-viewer/src/test/decorators-spec.ts

@@ -50,7 +50,7 @@ suite('decorators', () => {
      customElements.define(tagName, class extends StyleableElement {});

      element = document.createElement(tagName) as StyleableElement;
-      document.body.appendChild(element);
+      document.body.insertBefore(element, document.body.firstChild);


What's all this about? Something browser-specific?

Now that we are running in vanilla mocha, without additional intervention all of our elements get appended after the Mocha test results. This ensures that our test elements are inserted before those Mocha DOM elements.

elalish · 2020-03-10T21:19:01Z

packages/model-viewer/src/test/model-viewer-base-spec.ts

@@ -338,10 +338,10 @@ suite('ModelViewerElementBase', () => {

      test('sets a model within viewport to be visible', async () => {
        await until(() => {
-          return elements[0][$scene].visible;
+          return elements[2][$scene].visible;


Why this change?

Because we reversed the order that the elements are inserted to the document.

cdata · 2020-03-13T14:51:26Z

I'm happy to share that "full" unit test runs (including Chrome, 2 x Firefox, 2 x Desktop Safari, 2 x MS Edge, 2 x iOS Safari, and IE11 across both @google/model-viewer and @google/3dom) are finishing in ~8 minutes. Unit tests and fidelity tests both complete in under 10 minutes. This is a 55-65% reduction in test time compared to our Travis/WCT/Sauce builds (example build) even though we are running tests against additional browser targets.

elalish

Thank you for slogging through this, @cdata!

elalish · 2020-03-13T15:34:46Z

packages/model-viewer/scripts/ci-run-tests.sh

@@ -20,11 +20,8 @@ set -x

 if [ "${TEST_TYPE}" = "unit" ]; then

-  npm run test
+  npm run wct-local-chrome


Still confused by this name; are we really still using WCT?

Good catch, this script is vestigial and has been removed 👍

Chris Joel added 5 commits March 5, 2020 09:25

Use Karma to run <model-viewer> tests

0eca663

All the tests pass in Karma/BrowserStack

4c43afb

Kill WCT w/ fire, add working dev script

af3ece3

Fix test:legacy script

b1df980

Remove accidental global leak

6eb41e3

cdata changed the title ~~Use Karma to run <model-viewer> tests~~ Use Karma/BrowserStack to run <model-viewer> tests Mar 10, 2020

elalish previously approved these changes Mar 10, 2020

View reviewed changes

Fix overlapping test runs

dc131a9

cdata dismissed elalish’s stale review via dc131a9 March 10, 2020 21:29

Chris Joel added 4 commits March 11, 2020 09:36

Give up running BrowserStack in parallel

f3ec62a

Prevent fidelity tests from running in main build

67805d5

Add ci test scripts to top-level package.json

d311098

Run fidelity tests in Github Actions

518a8fd

cdata force-pushed the model-viewer-karma branch from 7ede272 to 518a8fd Compare March 11, 2020 18:04

Chris Joel added 13 commits March 11, 2020 11:19

Try a conditional

230291d

Don't cd in xvfb command; more conditional

b6db869

Move to GITHUB_WORKSPACE when running xvfb

ed6cf61

Try macos for fidelity tests

dbe659a

Direct path to lerna

f51c675

Stream fidelity test results

1174df8

Try XVFB again

cbffb27

Split PR build into separate config

1158cbf

Fix job names

a5a9a2b

Try a different condition

957f425

Update package-lock.json to purge Sauce deps

81ebd9d

Make sure USE_BROWSER_STACK is set

0a4e415

Remove Travis config

459ac03

elalish previously approved these changes Mar 13, 2020

View reviewed changes

Remove vestigial WCT config

4eaed2c

cdata dismissed elalish’s stale review via 4eaed2c March 13, 2020 15:57

elalish approved these changes Mar 13, 2020

View reviewed changes

cdata merged commit 74c707f into master Mar 13, 2020

cdata mentioned this pull request Mar 13, 2020

Deploy to Github Pages using Github Actions #1086

Closed

elalish added this to the v0.10.0 milestone Apr 15, 2020

elalish deleted the model-viewer-karma branch September 15, 2020 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Karma/BrowserStack to run <model-viewer> tests #1075

Use Karma/BrowserStack to run <model-viewer> tests #1075

cdata commented Mar 5, 2020 •

edited

Loading

elalish left a comment

elalish Mar 10, 2020

elalish Mar 10, 2020

elalish Mar 10, 2020

elalish Mar 10, 2020

elalish Mar 10, 2020

cdata Mar 13, 2020

elalish Mar 10, 2020

cdata Mar 13, 2020

elalish Mar 10, 2020

cdata Mar 13, 2020

elalish Mar 10, 2020

cdata Mar 13, 2020

elalish Mar 10, 2020

cdata Mar 13, 2020

elalish Mar 10, 2020

cdata Mar 13, 2020

cdata commented Mar 13, 2020 •

edited

Loading

elalish left a comment

elalish Mar 13, 2020

cdata Mar 13, 2020


		if [[ "${TRAVIS_PULL_REQUEST}" = "false" && "${TRAVIS_BRANCH}" != "master" ]]; then

Use Karma/BrowserStack to run <model-viewer> tests #1075

Use Karma/BrowserStack to run <model-viewer> tests #1075

Conversation

cdata commented Mar 5, 2020 • edited Loading

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdata commented Mar 13, 2020 • edited Loading

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdata commented Mar 5, 2020 •

edited

Loading

cdata commented Mar 13, 2020 •

edited

Loading